refactor(trie2): use more descriptive `felt.Hash` type instead of `felt.Felt` #3316

MaksymMalicki · 2025-11-28T13:33:32Z

This PR addresses #3282 (comment), and uses more descriptive felt.Hash type for trie node hash fields and felt.StateRootHash for all the state root hash fields in all triedb and trie2 packages

codecov · 2025-11-28T13:57:19Z

Codecov Report

❌ Patch coverage is 68.49315% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.18%. Comparing base (584e500) to head (ba21c5d).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
core/trie2/trie.go	28.57%	20 Missing ⚠️
core/felt/hash.go	33.33%	8 Missing ⚠️
core/trie2/trieutils/id.go	75.00%	6 Missing ⚠️
core/trie2/triedb/pathdb/journal.go	37.50%	5 Missing ⚠️
core/trie2/databasetest.go	70.00%	3 Missing ⚠️
core/trie2/trieutils/accessors.go	50.00%	2 Missing ⚠️
core/trie2/triedb/hashdb/database.go	94.11%	1 Missing ⚠️
core/trie2/triedb/rawdb/database.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3316      +/-   ##
==========================================
- Coverage   76.20%   76.18%   -0.02%     
==========================================
  Files         346      346              
  Lines       32690    32773      +83     
==========================================
+ Hits        24912    24969      +57     
- Misses       5987     6011      +24     
- Partials     1791     1793       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

core/trie2/triedb/hashdb/database_test.go

rodrigo-pino · 2025-12-05T14:14:44Z

core/trie2/triedb/pathdb/layertree_test.go

+	classNodes           map[felt.Hash]map[trieutils.Path]trienode.TrieNode
+	contractNodes        map[felt.Hash]map[trieutils.Path]trienode.TrieNode
+	contractStorageNodes map[felt.Hash]map[felt.Address]map[trieutils.Path]trienode.TrieNode


felt.Hash is the most general type, but I think it is being miss-used.

felt.Hash means that there is not a good type to define it and it makes sense to be a general type, for example the return type of hashing function.

On the other hand, here we know the semantic meaning each hash has here, so we should use the appropriate types.

As far as I understand, it should be:

classNodes keys should be of type felt.ClassHash

contractNodes should be of type felt.Address since it is a contract address

contractStorageNodes should be of type felt.Address to felt.StorageAddress (or felt.StorageSlot). <- The latter types are to be defined

I don't think its correct or I'm missing something here. In this particular example, The felt.Hash in the hashmap mimics the layertree, which maps state root -> trie nodeset. So the felt.Hash represents the state root. The trieutils.Path represents the class hash for class trie, address for contract trie and the storage slot for the contract storage tries

I see, then I think we should have a StateRootHash

core/trie2/triedb/pathdb/layertree_test.go

core/trie2/trie.go

core/trie2/triedb/pathdb/layertree_test.go

rodrigo-pino · 2025-12-05T21:05:10Z

core/trie2/triedb/pathdb/layertree_test.go


 	// Child to parent layer relationship
-	childToParent map[felt.Felt]felt.Felt
+	childToParent map[felt.Hash]felt.Hash


Similar coment to the previous ones, what would be the right hash here?

it's a mapping of child state root to the parent state root. Since its calculated from a few components using PoseidonHash (result of hashing function), I'd say it's correct or we introduce a special felt-like type for the state commitment

I think here we could use StateRootHash again

let me add it

EgeCaner

Looks good!

EgeCaner · 2025-12-09T11:26:06Z

core/trie2/trieutils/accessors.go

-func ReadStateID(r db.KeyValueReader, root *felt.Felt) (uint64, error) {
-	key := db.StateIDKey(root)
+func ReadStateID(r db.KeyValueReader, root *felt.StateRootHash) (uint64, error) {
+	key := db.StateIDKey((*felt.Felt)(root))


Should we make StateIDKey receive felt.StateRootHash?

If too big a change for this PR then @MaksymMalicki please leave a todo to update it

nope, good catch, added

rodrigo-pino · 2025-12-09T14:14:53Z

core/trie2/triedb/pathdb/cache_test.go

 func TestCacheMisses(t *testing.T) {
 	cache := newCleanCache(1024 * 1024)
-	owner := felt.FromUint64[felt.Address](1234567890)
+	owner := *felt.NewFromUint64[felt.Address](1234567890)


Why this change?

must have slipped during rebase, changed

rodrigo-pino · 2025-12-09T14:14:58Z

core/trie2/triedb/pathdb/cache_test.go


 	// Different owner
-	diffOwner := felt.FromUint64[felt.Address](9876543210)
+	diffOwner := *felt.NewFromUint64[felt.Address](9876543210)


And this one?

same, changed

rodrigo-pino · 2025-12-09T14:15:06Z

core/trie2/triedb/pathdb/cache_test.go


 var (
-	testOwner = felt.FromUint64[felt.Address](1234567890)
+	testOwner = *felt.NewFromUint64[felt.Address](1234567890)


rodrigo-pino · 2025-12-09T14:15:49Z

core/trie2/triedb/hashdb/database.go

-	contractRootNode, err := trienode.DecodeNode(contractRootBlob, contractRootHash, 0, contractClassTrieHeight)
+	contractRootNode, err := trienode.DecodeNode(
+		contractRootBlob,
+		(*felt.Felt)(contractRootHash),


A todo for DecodeNode saying that the contractRootHash should be updated

rodrigo-pino · 2025-12-09T14:16:33Z

core/trie2/triedb/hashdb/database_test.go

 	require.NoError(t, err)

 	owner := id.Owner()
 	nodeHash := node.Hash()


A todo for node.Hash() to make it's return type a felt.Hash

rodrigo-pino · 2025-12-09T14:16:46Z

core/trie2/triedb/hashdb/database_test.go

+	_, found := database.dirtyCache.getNode(
+		&owner,
+		path,
+		(*felt.Hash)(&nodeHash),


Same idea here

rodrigo-pino · 2025-12-09T14:17:36Z

core/trie2/triedb/hashdb/database_test.go

+			classRoot    felt.Hash
+			contractRoot felt.Hash


What kind of hashes are these?

they are dependent on the hashing function used by given trie, but those hashes are the hashed values of the children, so we either add something like felt.TrieNodeHash or we stick to the felt.Hash (which IMO is descriptive enough since the implementation of the trie is general purpose)

rodrigo-pino · 2025-12-09T14:18:22Z

core/trie2/triedb/hashdb/database_test.go

+				err := database.GetTrieRootNodes(
+				(*felt.Hash)(&classRootHash),
+				(*felt.Hash)(&contractRootHash),


Please refactor or add a todo for this function to change the parameters to the appropiate types

rodrigo-pino · 2025-12-09T14:21:21Z

core/trie2/trieutils/accessors.go

 	owner *felt.Address,
 	path *Path,
-	hash *felt.Felt,
+	hash *felt.Hash,


Is this Hash something more specific than felt.Hash

nope, it is the poseidon/pedersen hash of the children of this node. I think maybe some type like felt.TrieNodeHash would be okey, but generally imo felt.Hash is descriptive enough

MaksymMalicki force-pushed the maksym/refactor-triedb-address branch 2 times, most recently from 9210f6f to 090e07a Compare December 3, 2025 17:59

MaksymMalicki force-pushed the maksym/refactor-triedb-hash branch from ad10776 to 6bee396 Compare December 3, 2025 19:19

Base automatically changed from maksym/refactor-triedb-address to main December 4, 2025 05:22

MaksymMalicki force-pushed the maksym/refactor-triedb-hash branch from 6a421db to 886e905 Compare December 5, 2025 11:49

MaksymMalicki temporarily deployed to Development December 5, 2025 11:54 — with GitHub Actions Inactive

squash

96d28a1

MaksymMalicki force-pushed the maksym/refactor-triedb-hash branch from 886e905 to 96d28a1 Compare December 5, 2025 13:13

MaksymMalicki temporarily deployed to Development December 5, 2025 13:27 — with GitHub Actions Inactive

rodrigo-pino reviewed Dec 5, 2025

View reviewed changes

address comments

0f9e3c1

MaksymMalicki temporarily deployed to Development December 8, 2025 15:35 — with GitHub Actions Inactive

add StateRootHash type

6c936ee

MaksymMalicki temporarily deployed to Development December 8, 2025 19:17 — with GitHub Actions Inactive

rodrigo-pino requested review from EgeCaner and infrmtcs December 9, 2025 01:33

fix unit tests

ba21c5d

MaksymMalicki temporarily deployed to Development December 9, 2025 12:37 — with GitHub Actions Inactive

EgeCaner approved these changes Dec 9, 2025

View reviewed changes

rodrigo-pino reviewed Dec 9, 2025

View reviewed changes

address comments

187d4f9

MaksymMalicki deployed to Development December 9, 2025 22:02 — with GitHub Actions Active

refactor(trie2): use more descriptive felt.Hash type instead of felt.Felt #3316

Are you sure you want to change the base?

refactor(trie2): use more descriptive felt.Hash type instead of felt.Felt #3316

Uh oh!

Conversation

MaksymMalicki commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EgeCaner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaksymMalicki Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaksymMalicki Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

refactor(trie2): use more descriptive `felt.Hash` type instead of `felt.Felt` #3316

refactor(trie2): use more descriptive `felt.Hash` type instead of `felt.Felt` #3316

MaksymMalicki commented Nov 28, 2025 •

edited

Loading

codecov bot commented Nov 28, 2025 •

edited

Loading

MaksymMalicki Dec 9, 2025 •

edited

Loading

MaksymMalicki Dec 9, 2025 •

edited

Loading